Skip to content

Conversation

@judicaelandria
Copy link
Contributor

@judicaelandria judicaelandria commented Oct 10, 2025

Key Features

  • AirPlay Support: Safari/Apple device detection with Remote Playback API and WebKit fallback
  • Smart Detection: Shows AirPlay button only in Safari and on Apple devices
  • Automatic Integration: Uses videojs.hook for seamless plugin initialization without manual setup
  • State Management: Visual feedback for connecting/connected/disconnected states
  • Error Handling: User-friendly error messages and proper fallback mechanisms

Browser Compatibility

  • Safari/iOS/macOS: AirPlay button with Apple device targeting (Apple TV, other Macs, etc.)
  • Other browsers: No button shown (appropriate behavior)

Technical Implementation

  • TypeScript: Full type safety with comprehensive interfaces
  • Modular Architecture: Clean separation of concerns following Silvermine standards
  • Build System: Vite configuration for UMD/ESM outputs with bundled TypeScript declarations
  • Testing: Comprehensive test suite with proper mocking
  • Development: Interactive examples and dev server for real-time testing

Usage

// Simple integration - automatically works for all players
import VideoJsRemotePlayback from '[silvermine/videojs-remoteplayback';](cci:4://file://silvermine/videojs-remoteplayback';:0:0-0:0)
VideoJsRemotePlayback(videojs);

@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch 3 times, most recently from 8affb56 to dbdd8c6 Compare October 22, 2025 12:57
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch from dbdd8c6 to ac04633 Compare October 27, 2025 13:46
@joshuacurtiss
Copy link

joshuacurtiss commented Oct 30, 2025

Hello sir! Thank you for your work on this. Here are some observations with this PR. Like I mentioned IRL, I'm going to suggest that the code be reorganized a bit, so please bear with me here.

Problems

Some of these problems will be affected by my recommended code organizational changes, but I just wanted to clarify everything:

  • The project throws an error when building because RemotePlaybackPlugin.ts is pulling COMPONENT_NAMES from the wrong module.
  • The package-lock.json file is out of sync with package.json. Doing an npm install results in a modified package-lock.json. Please add this change to the commit where you change package.json.
  • Please use our standard indenting in package.json/package-lock.json.
  • Please lock in exact version numbers in package.json; do not use ^.
  • Please put only the built artifacts into dist. Tests and TS source are not needed.

Miscellaneous Improvements

  • Would you mind separating build-type of tasks into a separate commit? So, the additions to package.json/package-lock.json going into a separate commit helps tell a better story with the commits. The same principle would apply to Vite configurations.
  • Let's set up a dev server for serving our sample/test HTML pages. It's ok that these are separate from our tests directory, featuring vitetest unit tests. Doing this work in a separate test category commit would tell a good story.

Code Organization

This hierarchy may serve us well:

  • @types: Global, shared, or third-party type definitions
    • global.d.ts
    • videojs.d.ts
    • etc.
  • src
    • assets
      • image files, etc.
    • js
      • airplay
        • AirPlayManager.ts
        • AirPlayButton.ts
      • lib: Individual shared modules that meaningfully group shared functions together, for example:
        • safely-execute.ts
        • logging.ts
        • etc.
      • index.ts
      • registerPlugin.ts
      • RemotePlaybackPlugin.ts
      • etc.
    • styles
      • scss files, etc.

We want to avoid general files like utils.ts because the name provides no useful information and reduces discoverability. Even such files for a specific part of the project, like Airplay.functions.ts, unnecessarily reduce discoverability of the functions/constants/interfaces they contain, and usually are very clearly associated with a particular class that they can be added to as an export. In some cases, things need to be imported in from one of these files just to be used in a single file.

For instance:

  • AirPlayManager interface is clearly associated with AirPlayManager.ts
  • AirPlayButtonOptions interface is clearly assicated with AirPlayButton.ts
  • CSS_CLASSES is only used by AirPlayButton.ts
  • RemotePlaybackPlugin interface can be put in RemotePlaybackPlugin.ts
  • etc.

There may be some shared items that we would need to think about how to place them properly while abiding by this approach, but doing so will be a minor concern with this reorganization.

Please feel free to send me a message with questions. Thank you so much for enduring the long read!

@joshuacurtiss
Copy link

@judicaelandria: One additional tip regarding the test page: I suggest using the vjs-fluid class for these test pages, so they are mobile responsive.

I also noticed the links to the artifacts are incorrect, but that'll be resolved when you rework the demos to properly work in a Vite dev server environment.

@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch 2 times, most recently from 05a55ec to 37c6da1 Compare November 4, 2025 14:09
@judicaelandria judicaelandria changed the title feat: implement airplay support feat: Implements AirPlay functionality for Video.js with remoteplayback API Nov 4, 2025
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch 2 times, most recently from f6eaf42 to e748658 Compare November 13, 2025 19:38
@joshuacurtiss
Copy link

Thank you so much for all your work thus far. Here's some more feedback/concerns. Please be patient with this PR, as we're being very thorough since it will then set the pattern for the rest of the project. I have some other aspects of the project that I haven't been able to review yet but I wanted to at least get you this feedback for now.

  • Don't forget to run npm run standards to ensure all standardization tests are passing. Which in this case, they revealed:
    • A couple commits aren't following our commit conventions. Note for commits about dependencies, you can use the build type/category.
    • The stylelint script needs to point at the new styles directory.
  • When refactoring and moving some files around, instead of having a deletion commit where you "remove deprecated files", can you do a git mv so we can more easily see the flow of these files from the initial starter commit?

Build

  • Great decision to use vite-plugin-dts. We'll need to check that it's ok to use that before you merge. I'll ping you about that separately.
  • Please keep 3-char indentation on package.json and package-lock.json, and keep the author as Jeremy.
  • The dev and dev:build scripts reveal that there is a better way to run the dev server here... can we improve the dev server to actually use our src code, rather than the already-built artifacts. Then you can have true HMR while you develop. You can put the alternate vite config in the directory as well, like examples/vite.dev.config.ts.
    • Then dev and build serve clear purposes and a dev:build script is not needed.

Typing

  • videojs.ts
    • Instead of pulling VideoJsPlayer from 'video.js', would it be better to use videojs.Player, since you're already importing videojs from '@silvermine/video.js' and it's a proper class to extend for making your custom VideoJsPlayer interface?
    • Are you using VideoJsButton and VideoJsButtonConstructor?
  • Important: Organization of separate .interfaces.ts files. This does not follow our team's normal approach. Can we please put interfaces alongside the class definitions they reflect? For instance:
    • Airplay.interfaces.ts
      • ControlBarChild is not used
      • AirPlayManager can go in the AirPlayManager.ts file where the class is defined.
      • AirPlayButtonOptions can go in the AirPlayButton.ts file where it is used.
  • For constants, there's a little more leeway in how we use them, but we still should use the principle of "put it with the thing that uses it the most" when appropriate. For instance:
    • css-classes.ts: CSS_CLASSES can go in AirPlayButton.ts
    • component-names.ts: The constants in here are never reused, so probably unnecessary. If you do see them as potentially being reused for later work like the chromecast support, perhaps just move it to the main src/constants directory
    • remote-playback.ts
      • EVENTS: This is great use of constants that are heavily used throughout the plugin!
      • AVAILABILITY_STATES: This is only used in AirPlayButton.ts. Will it later be used by later work?
      • ARIA_ATTRIBUTES: Is this used?
    • plugin-name.ts: Used only once. Do we need it? At the very least can we move it to registerPLugin.ts where it is used the one time?
    • log-messages.ts: I have somewhat mixed feelings about this one. Most of these messages are only ever used once. Can we at least set them in the file that is using the message, instead of being stowed away in another constants file?
  • I know we didn't set it properly in the initial setup commit, but I believe tsconfig.json should extend @silvermine/typescript-config/browser/tsconfig.json so it is aware of various browser elements.

- Update vite.config.ts with library build settings for UMD and ESM outputs
- Add vite-plugin-dts for TypeScript declaration generation with bundled types
- Create vite.dev.config.ts for development server serving examples from dist
- Configure proper asset handling and CSS compilation for plugin distribution
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch from e748658 to bc13c98 Compare November 26, 2025 09:43
- Add @silvermine/video.js as peer dependency for plugin integration
- Configure package.json exports for proper UMD and ESM module resolution
- Update package metadata for remote playback functionality
- Add necessary development dependencies for TypeScript and Vite build process
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch 3 times, most recently from b383d23 to 28888de Compare November 26, 2025 13:01
- Create comprehensive TypeScript type definitions for Video.js integration
- Add plugin constants for events, CSS classes, and configuration
- Implement utility functions for media element handling and logging
- Add AirPlay icon asset and SCSS styling following Silvermine standards
- Establish proper project structure with modular organization
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch from 28888de to 34c6d48 Compare November 26, 2025 13:24
- Add AirPlayButton component with proper Video.js Button inheritance
- Implement AirPlayManager with Remote Playback API and WebKit fallback
- Add Safari/Apple device detection to show button only when appropriate
- Create comprehensive state management for connection status visualization
- Include proper error handling with user-friendly feedback messages
- Add support for both standard and large button variants with labels
- Create RemotePlaybackPlugin class with proper lifecycle management
- Implement automatic plugin registration using videojs.hook for seamless integration
- Add Video.js control bar integration with proper button placement
- Enable automatic initialization for all Video.js players without manual setup
- Provide clean plugin architecture following Video.js best practices
- Create AirPlayManager tests with proper browser detection mocking
- Add enhanced Video.js mock supporting plugin architecture and hooks
- Test device availability detection and state management
- Validate proper error handling and user feedback scenarios
- Ensure plugin initialization and lifecycle work correctly
- Create interactive test pages for AirPlay, Chromecast, and Remote Playback API
- Add detailed README with usage instructions and browser compatibility matrix
- Include development server setup for real-time testing during development
- Provide clear installation and integration examples
- Add examples documentation with testing guidelines and troubleshooting
- Update vite.dev.config.ts with proper file serving permissions
- Add dev:build script that builds plugin before starting dev server
- Ensure dist files are properly served at root path (/videojs-remoteplayback.*)
- Fix development workflow to eliminate asset loading issues
- Maintain clean separation between development and production builds
@judicaelandria judicaelandria force-pushed the mandriamahandry/add-airplay-support branch from 34c6d48 to 1f26814 Compare November 30, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants